Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Gzip middleware #84

Merged
merged 1 commit into from
Mar 9, 2014
Merged

Add a Gzip middleware #84

merged 1 commit into from
Mar 9, 2014

Conversation

romanbsd
Copy link
Contributor

No description provided.

@mislav
Copy link
Contributor

mislav commented Feb 25, 2014

Neat. However, is the middleware necessary? net/http, for instance, does this transparently. I would look into our other adapters and see if they do so as well. If most of them do it, I wouldn't maintain such middleware.

@romanbsd
Copy link
Contributor Author

Well, it does handle it, in some ruby versions, sometimes, for some cases. For the best of my knowledge, it doesn't add the Accepts-Encoding header anyway. So yes, net/http, and em-http-request might uncompress the body, given the chance, but the behavior is far from being well defined. As a side note, we're using it with Faraday + net-http-persistent (which is backed by net/http).

@mislav
Copy link
Contributor

mislav commented Feb 25, 2014

net/http started transparently handling gzip in Ruby 1.9. It adds the accepts-encoding directive and handles gzipped responses. This only applies to GET though. We assume that the responses that are results of POST, PUT, DELETE, etc. are small enough to not benefit from compression.

Your middleware is good stuff, but it doesn't provide any benefits for people already using net/http or net-http-persistent. As for other HTTP clients, I don't know how they handle this. If most of them don't, then I would accept this middleware as a way to get compression no matter what adapter.

CI is failing because you need to add class-level documentation for Gzip, BTW.

@romanbsd
Copy link
Contributor Author

Well, I'm 100% sure that Faraday.get(url) using the net/http doesn't use gzip.

@mislav
Copy link
Contributor

mislav commented Feb 25, 2014

What makes you think so? Given a debugging HTTP proxy on port 8888:

$ http_proxy=localhost:8888 ruby -rfaraday -e 'Faraday.get("http://api.github.com")'

This is the actual HTTP request that net/http makes:

GET http://api.github.com/ HTTP/1.1
User-Agent: Faraday v0.8.9
Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3
Accept: */*
Connection: close
Host: api.github.com

Notice the Accept-Encoding directive.

@romanbsd
Copy link
Contributor Author

My mistake, I was referring to net-http-persistent:

Faraday.get 'http://m.yahoo.com/'
opening connection to m.yahoo.com...
opened
<- "GET / HTTP/1.1\r\nUser-Agent: Faraday v0.9.0\r\nAccept: */*\r\nConnection: keep-alive\r\nKeep-Alive: 30\r\nHost: m.yahoo.com\r\n\r\n"

@mislav
Copy link
Contributor

mislav commented Feb 25, 2014

Ah, alright. You're right that net-http-persistent doesn't do compression in Ruby 1.9. In Ruby 2.0+ it starts, though. Weird. I'm guessing this behavior is dependent on some internal net/http change in Ruby 2.0.

EM also supports transparent decompression. Excon and Patron don't seem to; at least not by default.

@romanbsd
Copy link
Contributor Author

Yeah, I've noticed the inconsistency... :-/

@romanbsd
Copy link
Contributor Author

I'm not sure why Travis is unhappy now. Something to do with 1.8 :(

@mislav
Copy link
Contributor

mislav commented Feb 26, 2014

Zlib::GzipReader.new(StringIO.new(env[:body]), :encoding => 'ASCII-8BIT')

Ruby 1.8 doesn't support encodings.

@stve
Copy link
Contributor

stve commented Feb 26, 2014

I recently needed to do this as well, using the typhoeus adapter, and ended up writing something fairly similar to what @romanbsd has. This seems like it'd be a helpful addition, although I can see how it does pose an issue where an adapter already implements gzip encoding.

@romanbsd
Copy link
Contributor Author

So, should I go ahead and fix the 1.8 compat issue?

@mislav
Copy link
Contributor

mislav commented Mar 5, 2014

Yep, go ahead and fix that

@romanbsd romanbsd closed this Mar 5, 2014
@mislav
Copy link
Contributor

mislav commented Mar 5, 2014

Seems like you fixed it. CI is failing now because the call method is too complex. Breaking it down into smaller methods will help.

Did you intentionally close this?

@romanbsd
Copy link
Contributor Author

romanbsd commented Mar 5, 2014

yes, I created a new pull request. Breaking it down doesn't make a lot of sense, if you check the code. Is there another way to work around this draconian code style validator?

@mislav
Copy link
Contributor

mislav commented Mar 5, 2014

I would prefer if you just force-pushed to the same branch after rebasing instead of opening a new PR. That way we would have kept the discussion and the code in the same place.

If you don't want to change the code, you could add an exception to Cane:

cane --abc-exclude Foo::Bar#method

However, I think that the method can definitely be simplified a bit. I can take care of that if you think you're done with the code.

marknadig referenced this pull request in stavskiys/oauth2 Mar 5, 2014
@romanbsd romanbsd reopened this Mar 5, 2014
@romanbsd
Copy link
Contributor Author

romanbsd commented Mar 5, 2014

ok, force pushed. Closing the other pull request.

@romanbsd
Copy link
Contributor Author

romanbsd commented Mar 8, 2014

And by the way, arsduo/koala#346

@romanbsd
Copy link
Contributor Author

romanbsd commented Mar 8, 2014

I can't believe it, the travis CI build has finally passed 👍

@mislav mislav merged commit b7ef6e9 into lostisland:master Mar 9, 2014
@mislav
Copy link
Contributor

mislav commented Mar 9, 2014

Thanks, merged and cleaned up a bit in d35f960

@stve stve mentioned this pull request Jul 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants